Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use output_channel of externally triggered intent as input_channel #6272

Closed
wants to merge 5 commits into from
Closed

Use output_channel of externally triggered intent as input_channel #6272

wants to merge 5 commits into from

Conversation

pheel
Copy link
Contributor

@pheel pheel commented Jul 24, 2020

Proposed changes:

  • Currently any intent pushed to the tracker via /conversations/<conversation_id>/trigger_intent results in a user event with input channel None. This means latest output channel is effectively lost until a "real" user message comes in from a channel. This sets input channel using the output channel determined from the request.

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@sara-tagger
Copy link
Collaborator

Thanks for submitting a pull request 🚀 @b-quachtran will take a look at it as soon as possible ✨

@b-quachtran
Copy link
Contributor

@federicotdn It looks good to me, thoughts?

@federicotdn
Copy link
Contributor

It's a bit strange to be using an output channel to populate the latest input channel field of the tracker, given that they are not the same thing. Maybe we could update the tracker in a way where the latest input channel property is not lost? Maye @JEM-Mosig has an opinion on this.

@pheel
Copy link
Contributor Author

pheel commented Aug 25, 2020

@federicotdn, @b-quachtran, I could make rasa.core.trackers.DialogueStateTracker.get_latest_input_channel skip over messages with null output channel, and it would have the same effect of latest not losing its meaning. Sounds like a less invasive change. Should I go for it?

@pheel
Copy link
Contributor Author

pheel commented Aug 25, 2020

Ah, I see this was cloned at #6472.

@federicotdn
Copy link
Contributor

@pheel

I could make rasa.core.trackers.DialogueStateTracker.get_latest_input_channel skip over messages with null output channel, and it would have the same effect of latest not losing its meaning. Sounds like a less invasive change. Should I go for it?

I think that makes a lot of sense, given the name of the method get_latest_input_channel. The thing is, that with that change we would be effectively modifying the value the method returns in different contexts (on trackers with trailing UserUttered events with no input channel). The PR I opened was a test to see if it made sense to instead make the change more specific to the externally triggered intents feature (note that I did not change get_latest_input_channel). Apart from this, I still think we should not mix input and output channels.

@pheel
Copy link
Contributor Author

pheel commented Aug 26, 2020

Yeah, I'll close this PR in favor of #6472 then.

@pheel pheel closed this Aug 26, 2020
@pheel pheel deleted the output_channel_to_input_channel branch August 26, 2020 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants